Repository navigation
fix: reject a timezone-aware bound when the time column is naive - #331
Open
Muneeb Anjum (horizonbymuneeb) wants to merge 1 commit into
Open
Muneeb Anjum (horizonbymuneeb) wants to merge 1 commit into
Muneeb Anjum (horizonbymuneeb) wants to merge 1 commit into
Conversation
`fill_gaps` / `id_time_grid` accepted a timezone-aware `start` or `end` for a
timezone-naive pandas time column and silently converted the bound to UTC,
dropping the offset. A bound written as local midnight therefore landed on the
previous day:
df ds = 2024-01-02 .. 2024-01-06 (naive)
start = pd.Timestamp("2024-01-01", tz="Asia/Tokyo")
fill_gaps(df, "D", start=start)["ds"].iloc[0] # 2023-12-31
The grid then started a period early and the original values came back as NaN
under the generic "some values were lost" warning, so the bound looked usable and
the loss looked like a frequency problem.
The polars path already refuses this, because there is no way to know which zone
the naive column is in. The pandas path now raises the same `ValueError` before
the column is converted, so both backends agree. A tz-aware column with a tz-aware
bound keeps working, and naive bounds are untouched.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fill_gaps/id_time_gridaccepted a timezone-awarestartorendfor a timezone-naive pandas time column and silently converted the bound to UTC, dropping the offset.Root cause
_determine_bounddidbound.tz_convert("UTC").tz_localize(None)for any tz-awarepd.Timestamp, without asking whether the time column has a timezone at all. Converting local midnight in Tokyo to UTC gives2023-12-31 15:00, which then gets truncated to2023-12-31for a daily grid — one period before the date the user wrote. The original rows no longer line up with the grid, so they come back as NaN under the generic "some values were lost during filling" warning. The bound looks usable and the loss looks like a frequency problem, which is what makes it hard to diagnose.The polars path refuses the same input already, because there is no way to know which zone a naive column is in.
Fix
_validate_bound_pdraises the equivalentValueErrorfrom the pandas branch ofid_time_grid, before the column is converted to UTC. Only apd.Timestampcarrying a timezone on a naive column trips it —"per_serie","global", date strings and integers are untouched, and a tz-aware column with a tz-aware bound still works as before.The error message names the argument (
startorend) as well as the dtype, since both bounds are validated at the same place.Tests
tests/test_preprocessing.pygainsTestFillGapsTimezoneBounds:test_tz_aware_bound_on_naive_column_raisesstartandendboth raise, parametrizedtest_message_names_the_column_dtypetest_naive_bounds_on_naive_column_still_worktest_tz_aware_column_keeps_workingMutation check: with the fix stashed, three of these fail (both parametrizations of the raise test, plus the message test); with the fix, all five pass.
Test results
pytest tests/test_preprocessing.py -k TimezoneBoundspytest tests/(minustest_evaluation.py, which needs pyspark)The 5 failures are pre-existing in this environment and identical before and after the change —
test_processing.pypolars-version drift (pl.Expr,AttributeError) and atest_validation.pydatetime-dtype assertion. Comparing the two failure sets directly gives an empty difference, and the pass count rises by exactly the 5 tests added here.ruff checkis clean on both files. I did not runruff formatacross the test file: it is not ruff-formatted at its default width onmaineither, so reformatting it would bury a 45-line addition in unrelated churn.Compatibility
This changes behaviour only for input that previously returned a silently wrong grid: a tz-aware bound against a naive column now raises instead. Code that passed such a bound and did not notice the shifting grid will see the error, which is the point. Everything else — naive bounds, tz-aware columns, integer and polars paths — is unchanged.